Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add debug assertions to write_bytes and copy* #62103

Merged
merged 3 commits into from
Jul 16, 2019
Merged

Add debug assertions to write_bytes and copy* #62103

merged 3 commits into from
Jul 16, 2019
+57 −23

Conversation

RalfJung
Copy link
Member

Looks like @nitnelave went MIA in #58783, so I am re-submitting their PR, tweaked just a bit. I took care to preserve commit authorship.

Cc #53871

@rust-highfive
Copy link
Collaborator

r? @alexcrichton

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 24, 2019
@RalfJung RalfJung changed the title Debug assert Add debug assertions to write_bytes and copy* Jun 24, 2019
@RalfJung RalfJung mentioned this pull request Jun 24, 2019
Closed
@rust-highfive

This comment has been minimized.

Sign in to view
Centril
Centril reviewed Jun 25, 2019
View reviewed changes
src/libcore/intrinsics.rs Outdated Show resolved Hide resolved
src/libcore/intrinsics.rs Show resolved Hide resolved
src/librustc_codegen_llvm/metadata.rs Outdated Show resolved Hide resolved
@alexcrichton
Copy link
Member

Seems reasonable to me! r=me with @Centril's nits

@RalfJung
Copy link
Member Author

@alexcrichton I had to disable some codegen tests in debug mode, because even with optimizations the new debug assertions get in the way.

@Centril Centril mentioned this pull request Jun 25, 2019
Closed
@RalfJung
Copy link
Member Author

So the nits are fixed, but I'd like to get explicit confirmation that it is okay to ignore some (more) codegen tests in debug mode. We do run some CI runners in release mode, right?
(These are not the first codegen tests to be disabled in debug mode.)

@alexcrichton
Copy link
Member

@bors: r+

@bors
Copy link
Contributor

bors commented Jun 26, 2019

📌 Commit d3e1bf9 has been approved by alexcrichton

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 26, 2019
Centril added a commit to Centril/rust that referenced this pull request Jun 26, 2019
@Centril
Rollup merge of rust-lang#62103 - RalfJung:debug-assert, r=alexcrichton
adb4d4a 
Add debug assertions to write_bytes and copy*

Looks like @nitnelave  went MIA in rust-lang#58783, so I am re-submitting their PR, tweaked just a bit. I took care to preserve commit authorship.

Cc rust-lang#53871
@Centril Centril mentioned this pull request Jun 26, 2019
Closed
Centril added a commit to Centril/rust that referenced this pull request Jun 27, 2019
@Centril
Rollup merge of rust-lang#62103 - RalfJung:debug-assert, r=alexcrichton
e14989b 
Add debug assertions to write_bytes and copy*

Looks like @nitnelave  went MIA in rust-lang#58783, so I am re-submitting their PR, tweaked just a bit. I took care to preserve commit authorship.

Cc rust-lang#53871
@Centril Centril mentioned this pull request Jun 27, 2019
Closed
Centril added a commit to Centril/rust that referenced this pull request Jun 27, 2019
@Centril
Rollup merge of rust-lang#62103 - RalfJung:debug-assert, r=alexcrichton
215e422 
Add debug assertions to write_bytes and copy*

Looks like @nitnelave  went MIA in rust-lang#58783, so I am re-submitting their PR, tweaked just a bit. I took care to preserve commit authorship.

Cc rust-lang#53871
@Centril Centril mentioned this pull request Jun 27, 2019
Closed
Manishearth added a commit to Manishearth/rust that referenced this pull request Jul 2, 2019
@Manishearth
Rollup merge of rust-lang#62103 - RalfJung:debug-assert, r=alexcrichton
7982092 
Add debug assertions to write_bytes and copy*

Looks like @nitnelave  went MIA in rust-lang#58783, so I am re-submitting their PR, tweaked just a bit. I took care to preserve commit authorship.

Cc rust-lang#53871
@Manishearth Manishearth mentioned this pull request Jul 2, 2019
Closed
Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this pull request Jul 3, 2019
@Mark-Simulacrum
Rollup merge of rust-lang#62103 - RalfJung:debug-assert, r=alexcrichton
3b3ff6e 
Add debug assertions to write_bytes and copy*

Looks like @nitnelave  went MIA in rust-lang#58783, so I am re-submitting their PR, tweaked just a bit. I took care to preserve commit authorship.

Cc rust-lang#53871
@Mark-Simulacrum Mark-Simulacrum mentioned this pull request Jul 3, 2019
Closed
@RalfJung
Copy link
Member Author

RalfJung commented Jul 6, 2019

Building libcore with debug assertions works just fine though locally for this PR.

@mati865
Copy link
Contributor

mati865 commented Jul 6, 2019 •
edited
Loading

__aeabi_memcpy4 is only defined on arm and calls ptr::read: https://github.com/rust-lang-nursery/compiler-builtins/blob/39ad0538f2cfbc81b62e4f0ea1ee7a2f95133094/src/arm.rs#L144
Then ptr::read calls

rust/src/libcore/ptr/mod.rs

Line 601 in b820c76

copy_nonoverlapping(src, tmp.as_mut_ptr(), 1);
where you have added assertions.

This together with bjron3 comment should explain it.

@bjorn3
Copy link
Member

bjorn3 commented Jul 6, 2019

I think adding a second -llibcompiler_builtins-*.rlib at the beginning of the linker invocation could fix this.

@RalfJung RalfJung mentioned this pull request Jul 7, 2019
Merged
@RalfJung
Copy link
Member Author

RalfJung commented Jul 7, 2019

__aeabi_memcpy4 is only defined on arm and calls ptr::read

I see, that makes sense.

I think adding a second -llibcompiler_builtins-*.rlib at the beginning of the linker invocation could fix this.

I am afraid I have no idea how linkers are invoked in general and in Rust specifically. My current inclination is to add some cfg so that ptr::read only does these checks on x86, but that would be sad. Since you seem to have a pretty clear idea for what it takes, could you open a PR against https://github.com/RalfJung/rust/tree/debug-assert ?

@nikic
Copy link
Contributor

nikic commented Jul 7, 2019

@RalfJung Replacing ptr::write(dest, ptr::read(src)) with *dest = *src would probably do it as well.

@RalfJung
Copy link
Member Author

RalfJung commented Jul 7, 2019

So is there some kind of guarantee that the arguments of __aeabi_memcpy4 are 4-aligned?

@nikic
Copy link
Contributor

nikic commented Jul 7, 2019

@RalfJung Yes, that's exactly the difference between memcpy and memcpy4, the latter has guaranteed 4-byte alignment.

@alexcrichton
Copy link
Member

I would personally prefer to avoid updating any linkage with compiler-builtins, in general compiler-builtins should never be referencing panicking symbols. I agree with @nikic that the best solution for this is probably source-level changes in the compiler-builtins crate to avoid these assertions getting embedded, even in release mode.

bors added a commit that referenced this pull request Jul 9, 2019
@bors
Auto merge of #62460 - RalfJung:llvm-null, r=eddyb
8895384 
 Handle null from LLVMRustGetSectionName

As part of #58783 and #62103, this incorrect use of a NULL pointer was found in the interface to LLVM. That PR is stuck with some linker issues, but there is no reason the soundness fix should have to wait for that.
@RalfJung RalfJung mentioned this pull request Jul 13, 2019
Merged
@RalfJung
Copy link
Member Author

All right, submitted PR for compiler-builtins: rust-lang/compiler-builtins#300.

@RalfJung
Copy link
Member Author

I rebased the PR and added a commit that bumps compiler_builtins to the fresh 0.1.17 release.

@alexcrichton
Copy link
Member

@bors: r+

@bors
Copy link
Contributor

bors commented Jul 15, 2019

📌 Commit 85d76a1 has been approved by alexcrichton

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 15, 2019
Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this pull request Jul 15, 2019
@Mark-Simulacrum
Rollup merge of rust-lang#62103 - RalfJung:debug-assert, r=alexcrichton
f940617 
Add debug assertions to write_bytes and copy*

Looks like @nitnelave  went MIA in rust-lang#58783, so I am re-submitting their PR, tweaked just a bit. I took care to preserve commit authorship.

Cc rust-lang#53871
@Mark-Simulacrum Mark-Simulacrum mentioned this pull request Jul 15, 2019
Merged
bors added a commit that referenced this pull request Jul 16, 2019
@bors
Auto merge of #62704 - Mark-Simulacrum:rollup-oi94s2j, r=Mark-Simulacrum
02785da 
Rollup of 14 pull requests

Successful merges:

 - #62103 (Add debug assertions to write_bytes and copy*)
 - #62405 (Remove never_type feature requirement for exhaustive patterns)
 - #62491 (Fix Pin urls in Option documentation)
 - #62533 (Add missing links for CannotReallocInPlace type)
 - #62634 (Less unsafe in the array example of MaybeUninit docs)
 - #62639 (Make VaListImpl<'f> invariant over the 'f lifetime)
 - #62646 (Tweak wording in feature gate errors)
 - #62662 (add spaces in front of trait requirements in libcore/cell.rs)
 - #62668 (Fix #62660)
 - #62673 (miri validation: better error messages for dangling references)
 - #62680 (Actually call `visit_block_entry` in `DataflowResultsConsumer`)
 - #62685 (Add info about undefined behavior to as_ref suggestions)
 - #62689 (Fix typo in RawWaker::new documentation)
 - #62698 (SGX target: don't pretend to be GNU/Linux to LLVM)

Failed merges:

r? @ghost
@bors bors merged commit 85d76a1 into rust-lang:master Jul 16, 2019
@RalfJung RalfJung mentioned this pull request Jul 20, 2019
Merged
@RalfJung RalfJung deleted the debug-assert branch August 9, 2019 16:43
@saethlin saethlin mentioned this pull request Feb 9, 2024
Open
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Centril Centril Centril left review comments

Assignees

@alexcrichton alexcrichton

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.